change: [M3-6843] - Move manual snapshot error message to snapshot confirmation dialog #10791
Conversation
…ps page to snapshot confirmation dialogue
| import { BackupsPlaceholder } from './BackupsPlaceholder'; | ||
| import { BackupTableRow } from './BackupTableRow'; |
There was a problem hiding this comment.
these are just eslint changes (same with line 75 of CaptureSnapshot.tsx)
| @@ -5,7 +5,7 @@ import { ConfirmationDialog } from 'src/components/ConfirmationDialog/Confirmati | |||
| import { Typography } from 'src/components/Typography'; | |||
|
|
|||
| interface Props { | |||
| error?: string; | |||
| error: string | undefined; | |||
There was a problem hiding this comment.
there's no change in type/functionality of this prop, just made it non optional bc the one place we use this component now passes in an error
coliu-akamai
changed the title
coliu-akamai
requested review from
dwiley-akamai,
hkhalil-akamai and
mjac0bs
and removed request for
a team
|
Coverage Report: ✅ |
There was a problem hiding this comment.
Tested and looks good! Left one suggestion about using TransitionProps instead of useEffect.
several potential ways to display the snapshot error in the confirmation dialog
I agree with the using the first way for consistency, but we could check with UX if you feel like the other pattern is more appropriate. It may be out of scope of this ticket though.
packages/manager/src/features/Linodes/LinodesDetail/LinodeBackup/CaptureSnapshot.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks for adding this test!!!
There was a problem hiding this comment.
Good question about the positioning of the error notice. We're inconsistent with this.
Searching <ConfirmationDialog in the codebase, there are several that places we let the ConfirmationDialog format it below (as shown here) and several other places where we put the error first. (ConfirmTransferCancelDialog, ImagesLanding, DeletionDialog are a few).
It does seem like we go with Option 1 the most and for that reason, I would suggest sticking to that here... but it might be worth confirming with UX if we want to continue to go that route or make this consistent with one approach throughout our confirmation dialogs (in a separate ticket). Personally, I find it difficult to read, especially in dark mode, because the error message is so small and the color contrast is poor.
Another note: This Linode has nothing to backup, please add a disk and/or and a config is grammatically incorrect and really should be fixed on the backend. Could we let the API team know about this? (They may be fine with just a ticket since it looks like it's an error message coming from them and not some other service through APIv4.)
...r/src/features/Linodes/LinodesDetail/LinodeBackup/CaptureSnapshotConfirmationDialog.test.tsx
Show resolved
Hide resolved
...r/src/features/Linodes/LinodesDetail/LinodeBackup/CaptureSnapshotConfirmationDialog.test.tsx
Outdated
Show resolved
Hide resolved
|
Created internal ticket [M3-8453] + will add to frontend cafe discussion too! |
There was a problem hiding this comment.
Code review ✅
Unit test passes ✅
Error message appears in confirmation dialog now ✅
My personal preference would be for the second of the three possible display options (error notice dialog body text), but connecting with UX to set a convention would be great
mjac0bs
added
Approved
Description 📝
Moves the manual snapshot error message from the Linode Backups page to the snapshot's confirmation dialog
Target release date 🗓️
n/a
Preview 📷
Note - three potential options for displaying the error message - see "Other notes/questions" for more
How to test 🧪
Other notes/questions
the restmost of the confirmation dialogsAs an Author I have considered 🤔
Check all that apply